Skip to content

Conversation

rafaelcepeda
Copy link
Collaborator

Performed the necessary changes to enable compiler flag -require-explicit-sendable.

Copy link

linux-foundation-easycla bot commented Jul 14, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

@glbrntt glbrntt added the 🆕 semver/minor Adds new public API. label Jul 14, 2025
Package.swift Outdated
.enableUpcomingFeature("ExistentialAny"),
.enableUpcomingFeature("InternalImportsByDefault"),
.enableUpcomingFeature("MemberImportVisibility"),
.unsafeFlags(["-Xfrontend", "-require-explicit-sendable"]),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't unconditionally add this flag here; these flags are always enabled. This one should only be added for development.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forgot to remove it! Thanks for the reminder.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can still have it so we can enable it during development though - here's an example: https://github.com/apple/swift-nio-ssl/blob/main/Package.swift#L59-L73

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, let me add this then.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to add it back behind a flag. It was helpful when we were doing lots of strict concurrency work, but we're very unlikely to flip that flag back on now that the work has been done.

@rafaelcepeda rafaelcepeda requested a review from glbrntt July 14, 2025 13:00

@usableFromInline
struct AsyncIterator: AsyncIteratorProtocol {
struct AsyncIterator: AsyncIteratorProtocol, Sendable {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general, iterators aren't Sendable

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I mark it as not Sendable even though it currently conforms to Sendable then?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fact it implicitly conforms to Sendable right now because its implementation ticks all the boxes doesn't mean it should be Sendable when you consider the semantics. In general, all iterators keep some internal state to know what the next item should be. If an iterator is shared between different tasks, then it becomes undeterministic which task gets which element, which isn't good.

Also, I left the comment on this iterator but this should be true for all other iterators marked as Sendable on this PR.

@rafaelcepeda rafaelcepeda requested a review from gjcairo July 14, 2025 14:26
@rafaelcepeda rafaelcepeda merged commit 3fdc52d into grpc:main Jul 25, 2025
36 checks passed
@rafaelcepeda rafaelcepeda deleted the explicit_sendable branch July 25, 2025 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🆕 semver/minor Adds new public API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants